Update typings. Add typescript example. Cleanup package.json #41

evanshortiss · 2017-11-21T23:39:02Z

See notes in the PR for more information. This is aimed at making some improvements to TS support/docs/typings.

…external grunt

evanshortiss · 2017-11-21T23:39:31Z

.editorconfig

@@ -0,0 +1,11 @@
+# Tells the .editorconfg plugin to stop searching once it finds this file


This should help keep things more standardised across folks machines.

evanshortiss · 2017-11-21T23:40:26Z

.travis.yml

  - "4.4.3"
+  - "6"


We should be testing against recent versions of Node.js. I kept 4.4.3 since I believe we have some requirements around supporting that explicit version

Good spot! Let's remove 0.10 and 4 - it's gone for long time.

evanshortiss · 2017-11-21T23:40:47Z

.travis.yml

 services:
  - docker
 before_install:
  - sudo apt-get update
  - sudo apt-get install --assume-yes apache2-utils
  - npm install -g [email protected]
-  - npm install -g grunt-cli


Removed this since it's now in "devDependencies"

👎 it is for the CI process to run the script and test the project to allow the PR is required the grunt-cli

evanshortiss · 2017-11-21T23:41:37Z

fh-sync.d.ts

+      /**
+       * Options that can be passed to sync.invoke
+       */
+      interface InvokeOptions {


Added this so we can add better typings to the sync.invoke func

evanshortiss · 2017-11-21T23:42:16Z

fh-sync.d.ts

+      /**
+       * Valid actions (the "fn" param) that can be passed to sync.invoke
+       */
+      type InvokeAction = 'sync'|'syncRecords'|'listCollisions'|'removeCollision'


Using this will allow TS to flag if someone has a typo or wrong action in their sync.invoke options

evanshortiss · 2017-11-21T23:44:43Z

fh-sync.d.ts

+      /**
+       * Interfaces that describe how sync responses should be structured
+       */
+      namespace HandlerResults {


By having these defined and used by the callbacks for handlers we can have TS notify a developer that they're returning the correct/incorrect data structure.

For example:

sync.handleList('items', (dataset, query, meta, done) => { done(null, [1,2,3]) // this will not compile with the new changes done(null, { 431321: { /* data goes here */ } }) // this will compile since it's a hash as required })

evanshortiss · 2017-11-21T23:54:36Z

package.json

  "description": "FeedHenry Data Synchronization Server",
-  "main": "index.js",
+  "main": "fh-sync.js",
+  "files": [


This reduces installation size by removing examples, Gruntfile, etc. and only including essentials

evanshortiss · 2017-11-21T23:55:03Z

package.json

-    "grunt-cli": "^1.2.0",
-    "grunt-fh-build": "^0.5.0",
-    "grunt-mocha-test": "^0.13.2",
+    "grunt": "~0.4.5",


~ is safer than ^ in most cases so I locked these down a little

evanshortiss · 2017-11-22T00:34:56Z

fh-sync.d.ts

+       * Can be used by other modules to create pre-built sync compliant handlers.
+       */
+      namespace HandlerFunctions {
+          type Create = (dataset: string, data: Object, metaData: Object, done: StandardCb<HandlerResults.Create>) => void


This the format express typings use for middleware. It's useful since it allows creation of other libraries that can implement these types correctly, e.g sync adapters for specific services

import * from sync as 'fh-sync' import * from http as 'http-lib' const list: sync.HandlerFunctions.List = (dataset, query, meta, done) => { http.get(`http://myservice.acme.com/${dataset}`) .then(res => done(null, res)) .catch(done) } export = { list }

Then use like so

import * from sync as 'fh-sync' import * from acme as 'acme-sync-wrapper' sync.handleList('items', acme.list)

Up for debate if this is useful or not, but it cleans the file up a little too.

I can dig it. Being able to reference the types of inner things has always been a pain point, especially when it's the type of an argument. (I think there was actually an active issue in Typescript's repo about how to reference the type of an argument to a function.)

evanshortiss · 2017-11-22T00:35:36Z

fh-sync.d.ts

+      /**
+       * Connect sync server to mongo and redis. Returns the MongoDB and Redis clients being used internally.
+       *
+       * @param mongoDBConnectionUrl - Unique id of the dataset (usually collection, table in your database)


Function comments are now uniform and display a little more consistently. Should we add more detail in comments?

evanshortiss · 2017-11-22T00:44:19Z

package.json

    "valid-url": "1.0.9"
  },
-  "types": "./types/fh-sync.d.ts",
+  "types": "fh-sync.d.ts",


I moved this due to trouble when making my example. I can move back and try again if we'd prefer to keep it in a subfolder.

wtrocki · 2017-11-22T10:03:11Z

.travis.yml

  - "4.4.3"
+  - "6"
+  - "8"


lts/* will be better here. It my suggest that we support 8 :)

@wtrocki not sure I follow, do you mean we can put "*" instead of "8"?

wtrocki · 2017-11-22T10:13:17Z

examples/basic-express-typescript/package.json

+    "test": "echo \"Error: no test specified\" && exit 1",
+    "start": "tsc && node server.js"
+  },
+  "author": "Evan Shortiss <[email protected]> (http://evanshortiss.com/)",


I'm not against that, but having personalized author in organization code seems to be antipatern.

@wtrocki that's a mistake. npm init filled this in with my default and I forgot to remove 😊

wtrocki · 2017-11-22T10:14:43Z

fh-sync.d.ts

+// Project: https://github.com/feedhenry/fh-sync
+// Maintainer [email protected]
+
+declare module SyncCloud {


I'm going to check if all dependent projects compile after this change.
We may also bump minor version.

wtrocki · 2017-11-22T10:18:36Z

package.json

@@ -1,8 +1,14 @@
 {
  "name": "fh-sync",
-  "version": "1.0.13",
+  "version": "1.0.14",


TypeScript community recommends to make minor patches for all non trivial changes in types.
Many people may have patch version classifiers for types and suddenly their project will stop compiling on master. It happened couple times for us. Best to do minor bump.

👍 Good call, will do!

wtrocki · 2017-11-22T10:20:06Z

Build is failing on node8. Let's remove it. I will need time to get that tested with RainCatcher and I think we can merge it then.

MikeyBurkman · 2017-11-22T13:09:09Z

.travis.yml

  - "4.4.3"
+  - "6"
+  - "8"
 services:
  - docker
 before_install:
  - sudo apt-get update
  - sudo apt-get install --assume-yes apache2-utils
  - npm install -g [email protected]


Should we still be using npm 2.x, even when running on node 6/8?

I think that's a fair question. Perhaps @wtrocki can confirm if there's a requirement from some users?

MikeyBurkman · 2017-11-22T13:10:01Z

examples/basic-express-typescript/route.ts

+import * as cors from 'cors'
+import * as sync from '../../fh-sync'
+
+const router = express.Router()


No semis? Daring.

I'm trying it out here too 😂

MikeyBurkman · 2017-11-22T13:13:38Z

examples/basic-express-typescript/tsconfig.json

+    "removeComments": true,
+    "preserveConstEnums": true,
+    "sourceMap": true,
+    "target": "es5"


"strict": true by any chance?

Will take a look at that 👍

MikeyBurkman · 2017-11-22T13:17:14Z

fh-sync.d.ts

+       * Can be used by other modules to create pre-built sync compliant handlers.
+       */
+      namespace HandlerFunctions {
+          type Create = (dataset: string, data: Object, metaData: Object, done: StandardCb<HandlerResults.Create>) => void


I can dig it. Being able to reference the types of inner things has always been a pain point, especially when it's the type of an argument. (I think there was actually an active issue in Typescript's repo about how to reference the type of an argument to a function.)

MikeyBurkman · 2017-11-22T13:18:50Z

fh-sync.d.ts

+       * Example: {strategy: 'exp', max: 60*1000},
+       */
+      interface PendingWorkerBackoff {
+          strategy: string;


Can the strategies for this also be a literal string type, like what we have for InvokeAction?

Good question. I'd need to read the source code, perhaps @david-martin can share some insight?

@MikeyBurkman do you have an example of what you mean?

For example type InvokeAction = 'sync'|'syncRecords'|'listCollisions'|'removeCollision'

Thanks @wtrocki
Looks like either 'fib' or 'exp' are possible

fh-sync/lib/worker.js

Lines 8 to 25 in 7ca3e60

var bo;

if (backOffOpts.strategy && backOffOpts.strategy.toLowerCase() === 'exp') {

bo = new backoff.ExponentialStrategy({initialDelay: backOffOpts.min, maxDelay: backOffOpts.max});

} else if (backOffOpts.strategy && backOffOpts.strategy.toLowerCase() === 'fib') {

bo = new backoff.FibonacciStrategy({initialDelay: backOffOpts.min, maxDelay: backOffOpts.max});

} else {

//if no valid value is given, just return the default minimum delay. Can be used to turn off backoff.

console.log('[info] no valid sync strategy found in the backoff options. Turning off backoff', backOffOpts);

bo = {

next: function() {

return backOffOpts.min;

},

reset: function() {

}

};

}

return bo;

So it looks like there's technically three values: strategy: 'fib'|'exp'|undefined;

MikeyBurkman · 2017-11-22T13:22:48Z

fh-sync.d.ts

+       * @param redisUrl - Redis connection URL
+       * @param callback - Callback that will be invoked once connections are setup
+       */
+      function connect(mongoDBConnectionUrl: string, mongoDBConnectionOption: any, redisUrl: string, callback: (err: any, mongoDbClient?: any, redisClient?: any) => void): void;


Would be awesome if we could get typings for mongoDbClient and redisClient :)

I thought about that too, but passed on it. I suppose there's nothing to stop us adding @types/mongodb and @types/redis and referencing those?

MikeyBurkman · 2017-11-22T13:24:06Z

fh-sync.d.ts

+       * @param options - Specific options to apply to this dataset
+       * @param callback - Callback that will be invoked once initialisation is complete
+       */
+      function init(datasetId: string, options: SyncInitOptions, callback: StandardCb<void>): void;


Should be NoRespCb I think

MikeyBurkman · 2017-11-22T13:24:59Z

fh-sync.d.ts

+       * @param options - Options to pass to the invocation
+       * @param callback - Function that will receive the invocation results
+       */
+      function invoke(datasetId: string, options: InvokeOptions, callback: (err: any, result: any) => void): void;


Probably should be StandardCb<any> for consistency, though it doesn't matter too much here. Mostly just narrows the type of err a little bit.

wtrocki · 2017-11-28T12:56:55Z

@evanshortiss Reviewed locally. Great contribution.
Some minor problems need to be resolved and we are good to go!

…pdate invoke and init callback types. add backoff strategy types. add strict mode to example

evanshortiss · 2017-12-07T23:42:12Z

@wtrocki updated to remove Node.js 0.10 and 8 (temporarily) from CI 👍

wtrocki · 2017-12-08T11:30:31Z

@evanshortiss Thanks for contribution.
Tried that locally

Checked if things still compile in projects that using types.
Checked examples.

Added 2 commits to fix minor issues. If you are ok with all I think we can merge it and publish your changes to npm.

MikeyBurkman · 2017-12-08T15:31:32Z

examples/basic-express-typescript/package.json

+    "@types/body-parser": "~1.16.8",
+    "@types/cors": "~2.8.3",
+    "@types/express": "~4.0.39",
+    "ts-node": "^3.3.0"


@wtrocki You don't want to make this change to ts-node. Or at least you definitely don't want to remove typescript as a dependency. Right now, ts-node will use whatever version of typescript is installed globally, and there's no guarantee that it's a compatible version, if it's installed at all. (Also, I don't think it's common to use ts-node on prod apps, so this wouldn't be ideal as a starting point.)

As an update, I found this regarding ts-node in production. Using ts-node in prod isn't too bad, but it's a bit slower to restart the app.

Good point. Reason why this worked for me is that I had typescript installed from previous npm install. I forgot that ts-node requires typescript to be available globally.
I made that change as original change failed to start: tsc compiler is not available.
Before making any change - @evanshortiss What aproach do you prefer?

I don't have typescript installed locally so npm start failed for me. I installed it locally via package.json (like what Evan had) and npm start worked fine, so pretty sure that's all that's necessary.

What version do you have installed globally? I think the missing types issue that my other comment was about might have to do with that, as I was able to revert your changes and it compiled fine. (And intellisense was correct.)

MikeyBurkman · 2017-12-08T15:42:41Z

examples/basic-express-typescript/lib/route.ts

+// All sync requests are performed using a HTTP POST
+router.post('/:datasetId', (req: express.Request, res: express.Response, next: express.NextFunction) => {
+  // Invoke action in sync for specific dataset
+  sync.invoke(req.params.datasetId, req.body, function(err: any, result: any) {


@wtrocki Any reason you changed the types of the callback parameters to any? The types seem to get inferred correctly, and in fact, any is a wider type than err is, which is string|Error|null in the typings.

I did not compile for me - were complaining about missing types :)

I think this might be a noImplicitAny issue? In any case, does it mean we need better typings for result here? It should always be Object right?

evanshortiss · 2017-12-08T19:55:42Z

@wtrocki looks good, the only thing is I might remove ts-node as per the comments you and @MikeyBurkman had. Safer that way.

MikeyBurkman · 2017-12-12T15:14:03Z

fh-sync.d.ts

+       * Can be used by other modules to create pre-built sync compliant handlers.
+       */
+      namespace HandlerFunctions {
+          type Create = (dataset: string, data: Object, metaData: any, done: StandardCb<HandlerResults.Create>) => void


Just noticed a slight inconsistency with these handler functions. The first argument for half of them is dataset and the other half have datasetId but I think they're all the same thing?

Good point. They should be consistent.

MikeyBurkman · 2017-12-12T15:14:45Z

fh-sync.d.ts

+          type ListCollisions = (datasetId: string, metaData: any, callback: StandardCb<{ [hash: string]: Object }>) => void
+          type RemoveCollision = (datasetId: string, collision_hash: string, metaData: any, callback: StandardCb<any>) => void
+          type Interceptor = (datasetId: string, interceptorParams: SyncInterceptParams, callback: NoRespCb) => void
+          type Hash = (data: any) => string


There are two types of hash function callbacks. The record one gets (datasetId: string, record: any) and the global hash one gets Array<any>

Oooh, well spotted! Will fix that.

@MikeyBurkman I think the (datasetId: string, record: any) is no longer valid. I have some TS code now like this that is working with these types:

const hashHandler: sync.HandlerFunctions.Hash = (t: Item) => { return `${t.name}${t.lastUpdated}` }

Our documentation states the signature is:

$fh.sync.setRecordHashFn(dataset_id, function generateHash(dataset_id, record){});

Based on this code the implementation I have is correct, so the docs might need updating.

Take a look at the code I'll push shortly

wtrocki · 2017-12-14T09:42:43Z

Triggering build again. It's failing due to npm issues.

evanshortiss added 3 commits November 21, 2017 15:29

add typescript example. cleanup definition comments. remove need for …

ced9f42

…external grunt

update ignores

6a2853e

add 'files' entry to package.json

e28633a

evanshortiss commented Nov 21, 2017

View reviewed changes

evanshortiss added 2 commits November 21, 2017 16:28

update d.ts param comments

36499a8

remove test

a1e21bd

evanshortiss commented Nov 22, 2017

View reviewed changes

evanshortiss requested a review from wtrocki November 22, 2017 05:19

wtrocki reviewed Nov 22, 2017

View reviewed changes

MikeyBurkman reviewed Nov 22, 2017

View reviewed changes

allow query and meta to have 'any' type since Object is too strict. u…

2f68a77

…pdate invoke and init callback types. add backoff strategy types. add strict mode to example

wtrocki added 2 commits December 8, 2017 11:25

Fix typescript issues

99a33fb

Fix missing global compiler

705dd8e

MikeyBurkman reviewed Dec 8, 2017

View reviewed changes

Add typescript

7529853

evanshortiss added 2 commits December 11, 2017 17:31

fix hash handler types

05e9fb8

fix hash fn signature

43d5400

MikeyBurkman reviewed Dec 12, 2017

View reviewed changes

update hashing signature for global hash. normalise datasetId param

4be8601

wtrocki closed this Dec 14, 2017

wtrocki reopened this Dec 14, 2017

camilamacedo86 self-requested a review April 30, 2018 13:41

Merge branch 'master' into ts

3dcdd4b

		@@ -0,0 +1,11 @@
		# Tells the .editorconfg plugin to stop searching once it finds this file

	var bo;
	if (backOffOpts.strategy && backOffOpts.strategy.toLowerCase() === 'exp') {
	bo = new backoff.ExponentialStrategy({initialDelay: backOffOpts.min, maxDelay: backOffOpts.max});
	} else if (backOffOpts.strategy && backOffOpts.strategy.toLowerCase() === 'fib') {
	bo = new backoff.FibonacciStrategy({initialDelay: backOffOpts.min, maxDelay: backOffOpts.max});
	} else {
	//if no valid value is given, just return the default minimum delay. Can be used to turn off backoff.
	console.log('[info] no valid sync strategy found in the backoff options. Turning off backoff', backOffOpts);
	bo = {
	next: function() {
	return backOffOpts.min;
	},
	reset: function() {

	}
	};
	}
	return bo;

Update typings. Add typescript example. Cleanup package.json #41

Are you sure you want to change the base?

Update typings. Add typescript example. Cleanup package.json #41

Conversation

evanshortiss commented Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanshortiss Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

evanshortiss Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Nov 28, 2017

evanshortiss commented Dec 7, 2017

wtrocki commented Dec 8, 2017

MikeyBurkman Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanshortiss commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Dec 14, 2017

evanshortiss commented Nov 21, 2017 •

edited

Loading

wtrocki Nov 22, 2017 •

edited

Loading

evanshortiss Nov 21, 2017 •

edited

Loading

evanshortiss Nov 21, 2017 •

edited

Loading

MikeyBurkman Dec 8, 2017 •

edited

Loading